Skip to content

fix(wallet): fully purge seed + key on wallet delete#621

Open
joshuakrueger-dfx wants to merge 4 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-delete-wallet-seed-residue
Open

fix(wallet): fully purge seed + key on wallet delete#621
joshuakrueger-dfx wants to merge 4 commits into
RealUnitCH:stagingfrom
joshuakrueger-dfx:joshua/fix-delete-wallet-seed-residue

Conversation

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator

@joshuakrueger-dfx joshuakrueger-dfx commented Jun 1, 2026

Summary

The user-facing "Delete Wallet" only cleared walletAccountInfos (WalletStorage.deleteWallet). The walletInfos row — which holds the AES-GCM-encrypted seed — and the mnemonic encryption key in flutter_secure_storage both survived. So after a user deletes their wallet, the full mnemonic remained recoverable on the device (resale / GDPR right-to-erasure risk); only the current-wallet pointer was cleared, masking the residue.

Fix (non-breaking — the account-only deleteWallet is preserved for the onboarding-regenerate flow, which deliberately relies on the seed row surviving):

  • WalletStorage.deleteWalletCompletely(id) — deletes accounts and the walletInfos seed row in a transaction (FK-safe).
  • SecureStorage.deleteMnemonicKey() — removes the AES-GCM key.
  • WalletRepository.purgeWallet(id) = deleteWalletCompletely + deleteMnemonicKey.
  • WalletService.deleteCurrentWallet now calls purgeWallet.

Verified the onboarding-regenerate path does not call deleteCurrentWallet/deleteWallet at runtime (it defers the DB write), so it is unaffected.

Test

  • purgeWallet removes the seed row and the mnemonic key.
  • deleteWallet (account-only) still leaves the row + key intact (regenerate contract guard).
  • The service delete now verifies purgeWallet is called and deleteWallet is not.

Verification (local, CI-equivalent, Flutter 3.41.6)

  • flutter analyze on the 6 changed files: No issues found.
  • All tests pass with the fix; the service test fails when deleteCurrentWallet is reverted to deleteWallet, confirming the regression is caught.

Fixes the S2 item of #612.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

joshuakrueger-dfx commented Jun 1, 2026

Why this PR shows a red ✗

Two checks were red; one is fixed here, the other is pre-existing:

  1. Coverage Floor Gate (real, fixed): the gate requires 100% scoped line coverage and this PR initially measured 99.9% — the new SecureStorage.deleteMnemonicKey() body was never executed because the repository test mocks SecureStorage. Fixed by adding a direct SecureStorage unit test that invokes it (commit c7ac4c4); re-running.

  2. Visual Regression (pre-existing, not this change): fails on exactly one golden —

    test/goldens/screens/home/home_golden_test.dart
  "HomePage loaded state with price data (variant: macOS)"  → 101 passed, 1 failed

    This drift also fails on other develop-based PRs (e.g. fix(kyc): collect countryAndTINs for non-Swiss residents + tighten phone validator #604) and on zero-UI changes, while develop-push is green. It needs a golden-regenerate.yaml baseline run to clear — separate from this fix.

Functional checks (Analyze & Test, BitBox quirks audit) are green.

This was referenced Jun 1, 2026
Open
Open
@TaprootFreak
Copy link
Copy Markdown
Contributor

TaprootFreak commented Jun 1, 2026

Per global repo convention (CLAUDE.md):

Don't reference the current task, fix, or callers ("used by X", "added for the Y flow", "handles the case from issue #123"), since those belong in the PR description and rot as the codebase evolves.

Please drop both kinds of references before merge:

Issue refs:

  • test/packages/repository/wallet_repository_test.dart — the // Regression for #612 S2: … block.
  • test/packages/service/wallet_service_test.dart — the (#612 S2) in the inline comment above the verify(() => repo.purgeWallet(8)) assertion.

Caller / siblng-method references (rot risk):

  • lib/packages/repository/wallet_repository.dart (docstring of purgeWallet) — [deleteWallet] (account-only) is preserved for onboarding regen.
  • lib/packages/service/wallet_service.dart — not the account-only deleteWallet used by onboarding-regenerate.
  • lib/packages/storage/secure_storage.dart (docstring of deleteMnemonicKey) — Call only from the user-facing wallet delete (purge) — … The onboarding-regenerate account-only delete must NOT call this.
  • lib/packages/storage/wallet_storage.dart (docstring of deleteWalletCompletely) — Distinct from [deleteWallet], which clears only accounts and deliberately leaves the seed row for the onboarding-regenerate flow.

These name the caller / sibling method and rot the moment any of them is renamed. If the contrast matters at the code level, it's worth renaming the surviving primitive (e.g. deleteWallet → something like clearWalletAccounts) so the method name itself carries the meaning. Otherwise the docstrings can collapse to one line that just states what the method does.

Fix itself looks correct (proper split between user-facing purge and onboarding-regenerate account-only delete, tests cover both paths).

@TaprootFreak
Copy link
Copy Markdown
Contributor

TaprootFreak commented Jun 1, 2026

Re API as Decision Authority (CONTRIBUTING.md:23–67): local on-device seed/key storage is the textbook example of a "physical security boundary, cannot be API-driven" per the rule. Both new primitives (purgeWallet, deleteMnemonicKey) operate purely on local SQLCipher rows and Keychain/EncryptedSharedPreferences. ✓

No API call involved; nothing for the backend to know about — the wallet's local cryptographic identity is gone after purge, future API calls will simply auth as a fresh wallet.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

joshuakrueger-dfx commented Jun 1, 2026

@TaprootFreak done — dropped the #612 S2 refs from both tests, and collapsed the four docstrings/comments to remove the caller/sibling-method references (purgeWallet, wallet_service, deleteMnemonicKey, deleteWalletCompletely). I went with the docstring-collapse path rather than renaming deleteWallet → clearWalletAccounts, to keep the change scoped to comments; happy to do the rename in a follow-up if you'd prefer the method name to carry the contrast. HomePage golden regenerated on dfx01, Visual Regression green.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

joshuakrueger-dfx commented Jun 1, 2026

Repo-rule compliance (commit 23e04ef): annotated the new deleteMnemonicKey secure-storage path with // @no-integration-test: <reason> per CONTRIBUTING.md. The unit test mocks FlutterSecureStorage (not a platform-interface fake), so the real Keystore/Keychain removal isn't exercised — the annotation makes that a deliberate, reviewable decision, matching the biometric_service_adapter.dart precedent.

@joshuakrueger-dfx
Copy link
Copy Markdown
Collaborator Author

joshuakrueger-dfx commented Jun 1, 2026

Status — ready for review

  • Review feedback addressed: dropped the #612 S2 issue refs from both tests and collapsed the four docstrings/comments to remove the caller/sibling-method references.
  • Repo rules: added the // @no-integration-test annotation for the new deleteMnemonicKey secure-storage path (23e04ef) — the unit test mocks the plugin, so the real Keystore/Keychain removal isn't exercised.
  • CI fully green on 23e04ef: Analyze & Test, Visual Regression, Coverage Floor Gate, BitBox quirks audit.

Open gates: formal maintainer approval (REVIEW_REQUIRED); on-device check of the keystore/keychain purge (not CI-verifiable).

@TaprootFreak TaprootFreak changed the base branch from develop to staging June 1, 2026 14:36
@joshuakrueger-dfx @TaprootFreak
fix(wallet): fully purge seed + key on wallet delete
9f185ff 
The user-facing 'Delete Wallet' only cleared walletAccountInfos via
WalletStorage.deleteWallet — the walletInfos row (the AES-GCM-encrypted seed)
and the mnemonic encryption key in secure storage both survived, leaving the
full mnemonic recoverable after a delete (resale / right-to-erasure risk).

Add a distinct purge primitive used only by the user-facing delete:
- WalletStorage.deleteWalletCompletely (accounts + the seed row, in a tx)
- SecureStorage.deleteMnemonicKey
- WalletRepository.purgeWallet = deleteWalletCompletely + deleteMnemonicKey
- WalletService.deleteCurrentWallet now calls purgeWallet
The account-only deleteWallet is left untouched for the onboarding-regenerate
flow (which relies on the seed row surviving).

Tests: purgeWallet removes the row AND the key; deleteWallet still leaves both;
the service delete now verifies purgeWallet (never deleteWallet).

Refs RealUnitCH#612 (S2)
@joshuakrueger-dfx @TaprootFreak
test(storage): cover SecureStorage.deleteMnemonicKey
d1efe4b 
The repository test mocks SecureStorage, so deleteMnemonicKey's real body was
never executed → scoped line coverage fell to 99.9% and the 100% Coverage Floor
Gate failed. Add a direct SecureStorage unit test that invokes it.
@joshuakrueger-dfx @TaprootFreak
fix(wallet): drop issue refs and caller/sibling-method refs from dele…
99b7cc8 
…te-purge comments
@joshuakrueger-dfx @TaprootFreak
docs(storage): annotate deleteMnemonicKey platform path with @no-inte…
d132e0a 
…gration-test

Per CONTRIBUTING.md, new secure-storage (platform-channel) code paths must
carry the // @no-integration-test annotation while no integration_test/ dir
exists; the unit test only mocks the plugin.
@TaprootFreak TaprootFreak force-pushed the joshua/fix-delete-wallet-seed-residue branch from 23e04ef to d132e0a Compare June 2, 2026 08:22
@TaprootFreak TaprootFreak added the tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joshuakrueger-dfx @TaprootFreak